internal: send a disconnect when key exchange fails - #1171
Conversation
There was a problem hiding this comment.
Pull request overview
This PR ensures wolfSSH sends an explicit SSH_MSG_DISCONNECT with reason WOLFSSH_DISCONNECT_KEY_EXCHANGE_FAILED when key exchange fails during KEXDH_INIT / KEXDH_REPLY, instead of silently dropping the transport. It also extends the duplex regression harness to mutate KEX packets and assert the disconnect reason is transmitted on the wire.
Changes:
- Add disconnect-on-fatal-KEX-failure guards to
DoKexDhInit()(server side) andDoKexDhReply()(client side). - Extend the regression duplex mutator to truncate
f(inKEXDH_REPLY) and truncate/emptye(inKEXDH_INIT), plus record outbound disconnect reason codes. - Add new regression cases asserting the disconnect reason is observed, and assert successful handshakes do not produce disconnects.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/internal.c |
Sends SSH_MSG_DISCONNECT with KEY_EXCHANGE_FAILED when KEX fails mid-exchange in both client/server KEX handlers. |
tests/regress.c |
Adds new KEX packet mutation modes and disconnect sniffing/assertions for KEX failure paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
09ad881 to
be9d8d2
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1171
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
be9d8d2 to
226e6ca
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1171
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
226e6ca to
33118c0
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1171
Scan targets checked: none
Failed targets: wolfssh-bugs, wolfssh-src
ejohnstown
left a comment
ejohnstown
left a comment
There was a problem hiding this comment.
I sent a change request directly.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1171
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
- DoKexDhInit() sends SSH_MSG_DISCONNECT with KEY_EXCHANGE_FAILED on WS_CRYPTO_FAILED and WS_PUBKEY_REJECTED_E, DoKexDhGexGroup() on WS_CRYPTO_FAILED and WS_DH_SIZE_E. - DoKexDhReply() sends KEY_EXCHANGE_FAILED on WS_CRYPTO_FAILED and HOST_KEY_NOT_VERIFIABLE on WS_PUBKEY_REJECTED_E. - DuplexEndpoint records the reason code of a plaintext outbound disconnect, and InitKexReplyHarnessKex() takes an explicit KEX algorithm. - New mutator modes shorten f and e, write a zero-length e, cut the GEX prime below the requested floor and set the GEX generator to 1; LocateSinglePacketPayload() finds the payload for the single-packet rewriters. - Tests assert the reason code on the wire for each new mode and for host key rejection, and assert no disconnect on a successful handshake. Issue: F-8838
33118c0 to
d80b37e
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1171
Scan targets checked: wolfssh-bugs, wolfssh-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
Problem
Key exchange failures aborted the handshake without transmitting
SSH_MSG_DISCONNECT, leaving the peer with a dropped transport and no reasoncode. RFC 8731 sec. 3 makes this normative for curve25519/448: clients and
servers "MUST also abort if the length of the received public keys are not the
expected lengths. An abort for these purposes is defined as a disconnect
(SSH_MSG_DISCONNECT) of the session and SHOULD use the
SSH_DISCONNECT_KEY_EXCHANGE_FAILED reason."
Fix (
src/internal.c)Three guards at KEX message boundaries:
DoKexDhInit()WS_CRYPTO_FAILED,WS_PUBKEY_REJECTED_E3KEY_EXCHANGE_FAILEDDoKexDhReply()WS_CRYPTO_FAILED3KEY_EXCHANGE_FAILEDDoKexDhReply()WS_PUBKEY_REJECTED_E9HOST_KEY_NOT_VERIFIABLEDoKexDhGexGroup()WS_CRYPTO_FAILED,WS_DH_SIZE_E3KEY_EXCHANGE_FAILEDWS_PUBKEY_REJECTED_EinDoKexDhReply()is only thepublicKeyCheckCbrejection — an application trust decision rather than a key agreement failure —
so it gets its own reason. The
(void)cast preventsSendDisconnect()'s ownstatus from masking the KEX error.
The finding's second half, an explicit 32-byte curve25519 length check, is
redundant:
wc_curve25519_check_public()already rejects any length other thanCURVE25519_KEYSIZEbefore doing anything else.Closes
f-8838.Tests (
tests/regress.c)Duplex-harness mutator modes, plus outbound-disconnect recording on both
endpoints. Seven cases assert the reason code reaches the wire:
fWS_CRYPTO_FAILEDeWS_CRYPTO_FAILEDeWS_PUBKEY_REJECTED_EWS_DH_SIZE_EWS_CRYPTO_FAILEDWS_PUBKEY_REJECTED_EWS_PUBKEY_REJECTED_EA successful handshake asserts no disconnect, so the tests cannot pass on a
detector that always fires. DH and ML-KEM are covered by construction: a
truncated DH mpint is still a valid group element.
Verification
gcc-13 -Werroracross 6 configs;make check11 pass, 1 skip.WS_DH_SIZE_Ealone, fails the matching test and only that test.Not in this PR
WS_RSA_E,WS_ECC_E,WS_ED25519_E,WS_MLDSA_E) still abort without a disconnect. No RFCmandates a disconnect there, and unlike the callback rejection above they are
cryptographic checks rather than an application trust decision.
DoKexDhGexRequest()→SelectKexDhGexGroup()→WS_DH_SIZE_E) is likewise silent. RFC 4419 sec. 3says only that "the key exchange fails", with no disconnect mandate, so this
is a consistency item tracked as a follow-up.